Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix HTTP parsing #433

Closed
wants to merge 3 commits into from
Closed

Fix HTTP parsing #433

wants to merge 3 commits into from

Conversation

ixti
Copy link
Member

@ixti ixti commented Oct 1, 2017

☣️ DO NOT MERGE ☣️

This is a discussion starter for resolving #422

ixti added 3 commits October 1, 2017 19:41
I have updated nodejs/http-parser dependency there. But the problem is
that JRuby most likely still affected :((
@ixti
Copy link
Member Author

ixti commented Oct 1, 2017

@httprb/core @janko-m @Bonias What do you think about us changing underlying http parser.

The problem with existing one is that it's locked to the old version of nodejs/http-parser. That can be fixed by updating underlying C version, but Java version of it is not maintained at all. So I think that the best option for us is to use FFI.

I'm not big fan of http-parser ffi wrapper but mostly due to it's API (it seems a bit ugly to me) and after all we end up by wrapper over wrapper. So probably if there are no strong objections against we can have our own http parser ffi version :D

@ixti
Copy link
Member Author

ixti commented Oct 1, 2017

Invited mastodon devs to participate in discussion as well.

//cc @Gargron

@Gargron
Copy link

Gargron commented Oct 1, 2017

cc @unarist @akihikodaki

@ixti
Copy link
Member Author

ixti commented Oct 1, 2017

So, the principal questions I propose to focus on are:

  • if there are any objections against FFI
  • if no objects, should we use existing gems for that or implement ourselves

@tarcieri
Copy link
Member

tarcieri commented Oct 1, 2017 via email

@akihikodaki
Copy link

I have no objections to FFI.
I suggest to use the existing gem. Though its API may look odd, it is just conformant with the underlying the library and legitimate as a language binding.
I do not think wrapper over wrapper is really bad in this case. You just have different wrappers playing different roles: a wrapper providing APIs convenient for http.rb and a language binding.

@tarcieri
Copy link
Member

tarcieri commented Oct 2, 2017

Curious what @tmm1 thinks about converting the upstream http_parser.rb gem to use FFI and dropping the extant JRuby extension

@zanker
Copy link
Contributor

zanker commented Oct 2, 2017 via email

@tarcieri
Copy link
Member

tarcieri commented Oct 2, 2017

@zanker yes, although it will build the native code library at the time you install the gem

@zanker
Copy link
Contributor

zanker commented Oct 2, 2017 via email

@ixti
Copy link
Member Author

ixti commented Oct 2, 2017

@akihikodaki Well, I guess I have expressed myself a bit wrong. I'm totally fine with using existing gems even if it leads to wrapper over wrapper. I have only 2 problems with that particular gem:

  • Naming. I find naming and some ruby API decisions pretty terrible. Not talking about things that are mapped to underlying library (Just general namespace and HttpParser::Parser.instance call makes me a bit sceptical)
  • Consumption of body. I did not found streaming API there it seems like it fires body in one chunk after it was consumed (in case of non-chunked responses). (I was wrong - see next comment)

@ixti
Copy link
Member Author

ixti commented Oct 2, 2017

Okay. Excuse me. I was wrong. on_body actually fired everytime there's some data in body arrived.

@akihikodaki
Copy link

akihikodaki commented Oct 2, 2017

Oh, I see. HttpParser::Parser, HttpParser::Instance, HttpParser::Parser.new_instance… Indeed that sound bad.

@janko
Copy link
Member

janko commented May 27, 2018

After finding out that http_parser.rb doesn't work on JRuby 9.2.0.0 anymore (#475), I realized why this PR is important 😃

It's great that there already exists the http-parser gem that builds upon Node.js' http-parser (just like the current http_parser.rb), but uses FFI instead of native extensions. I'm totally for using the http-parser gem, if any functionality is missing we can send PRs there.

Thanks @ixti for working on this!

@ixti
Copy link
Member Author

ixti commented Jan 14, 2019

Closed in favour of #489

@ixti ixti mentioned this pull request Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants